-
Notifications
You must be signed in to change notification settings - Fork 175
Add list of hosts with ports for setting connection #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add list of hosts with ports for setting connection #139
Conversation
Looks good to me. I think it's better to merge commits in one. And providing more information about the reasons for commit (fault tolerance) and about the implementation (round robin) in pull request description would be great |
The idea/motivation is clean. Maybe it would be clearer to push that logic a bit deeper? Here we have iteration through different IPs which can be extracted from the same domain name: clickhouse-cpp/clickhouse/base/socket.cpp Lines 87 to 88 in b36c87f
Maybe we can generalize that code a bit for multiple hosts/ports cases instead of creating one more retry loop outside? In that case, connect timeouts should work better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add proper unit-tests and fix highlighted issues.
/// List of hostnames with service ports | ||
struct HostPort { | ||
std::string host; | ||
std::optional<unsigned int> port; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix: add #include <optional>
Or enable editing PR by maintainers.
@@ -510,6 +510,67 @@ int main() { | |||
.SetCompressionMethod(CompressionMethod::LZ4)); | |||
RunTests(client); | |||
} | |||
|
|||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests instead, the sample program is not executed in CI/CD and hence your code is not checked. I suggest you to add a separate test-cases, both positive and negative in client_ut.cpp
:
{ | |
TEST(ClientCaseConnect, MultipleEndpoints) { | |
} |
struct HostPort { | ||
std::string host; | ||
std::optional<unsigned int> port; | ||
|
||
explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) { | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename HostPort
to Endpoint
(here and everywhere else), it is a bit more clear. Also, you can significantly simplify the struct definition:
struct HostPort { | |
std::string host; | |
std::optional<unsigned int> port; | |
explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) { | |
} | |
}; | |
struct Endpoint { | |
std::string host; | |
std::optional<unsigned int> port = std::nullopt; | |
} | |
}; |
explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) { | ||
} | ||
}; | ||
DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{}); | |
/** Set endpoints (host+port), only one is used. | |
* Client tries to connect to those endpoints one by one, on the round-robin basis: | |
* first default enpoint (set via SetHost() + SetPort()), then each of endpoints, from begin() to end(), | |
* the first one to establish connection is used for the rest of the session. | |
* If port part is not specified, default port (@see SetPort()) is used. | |
*/ | |
DECLARE_FIELD(endpoints, std::vector<Endpoint>, SetEndpoints, {}); |
@@ -296,73 +320,100 @@ void Client::Impl::Ping() { | |||
} | |||
|
|||
void Client::Impl::ResetConnection() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠ If there was a temporary communication problem, the client could re-connect to a different server (which I believe may be unexpected by the user).
Please make sure that client tries to reconnect to the same server first (options_.send_retries
times).
Also I suggest to explicitly allow users to reset connection endpoint, maybe via separate function or ResetConnection()
overload/parameter.
E.g.:
ResetConnection()
- try reconnecting to current endpoint.ResetConnectionEndpoint()
- try connecting to different endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in direct messages that:
ResetConnectionEndpoint()
should try to connect to different endpoints one by one only one time. If it doesn't work, throw an exception. It should be called on the client start and it should be called N
times. Also the user can call it in order to connect to different endpoint.
ResetConnection()
should try to reconnect to the current endpoint N times. It doesn't try to connect to other endpoints. If it doesn't work, throw an exception. So it's behaviour doesn't change
@@ -167,6 +190,7 @@ class Client::Impl { | |||
#endif | |||
|
|||
ServerInfo server_info_; | |||
std::optional<ClientOptions::HostPort> connected_host_port_; | |||
}; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please modify endpoints creation to simplify the ResetConnection()
ClientOptions modifyClientOptions(ClientOptions opts) | |
{ | |
if (!opts.host.empty()) | |
opts.endpoints.insert(opts.endpoints.begin(), ClientOptions::Endpoint{opts.host, opts.port}); | |
return opts; | |
} | |
Client::Impl::Impl(const ClientOptions& opts) | |
: options_(modifyClientOptions(opts)) | |
{ |
@@ -296,73 +320,100 @@ void Client::Impl::Ping() { | |||
} | |||
|
|||
void Client::Impl::ResetConnection() { | |||
connected_host_port_.reset(); | |||
for (int i = -1; i < int(options_.hosts_ports.size()); ++i) { | |||
const ClientOptions::HostPort& host_port = i == -1 ? ClientOptions::HostPort(options_.host) : options_.hosts_ports[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the list of endpoints before copying it to a options_
so you don't have to do this. (see above)
@@ -196,6 +206,8 @@ class Client { | |||
|
|||
const ServerInfo& GetServerInfo() const; | |||
|
|||
const std::optional<ClientOptions::HostPort>& GetConnectedHostPort() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify in comments when this function may return nullopt
.
ClientOptions::HostPort correct_host_port = ClientOptions::HostPort("localhost", 9000); | ||
Client client(ClientOptions() | ||
.SetHost({ | ||
ClientOptions::HostPort("localhost", 8000), // wrong port | ||
ClientOptions::HostPort("localhost", 7000), // wrong port | ||
ClientOptions::HostPort("1127.91.2.1"), // wrong host | ||
ClientOptions::HostPort("1127.91.2.2"), // wrong host | ||
ClientOptions::HostPort("notlocalwronghost"), // wrong host | ||
ClientOptions::HostPort("another_notlocalwronghost"), // wrong host | ||
correct_host_port, | ||
ClientOptions::HostPort("localhost", 9001), // wrong port | ||
ClientOptions::HostPort("1127.911.2.2"), // wrong host | ||
}) | ||
.SetPingBeforeQuery(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this allows simplifying client code:
ClientOptions::HostPort correct_host_port = ClientOptions::HostPort("localhost", 9000); | |
Client client(ClientOptions() | |
.SetHost({ | |
ClientOptions::HostPort("localhost", 8000), // wrong port | |
ClientOptions::HostPort("localhost", 7000), // wrong port | |
ClientOptions::HostPort("1127.91.2.1"), // wrong host | |
ClientOptions::HostPort("1127.91.2.2"), // wrong host | |
ClientOptions::HostPort("notlocalwronghost"), // wrong host | |
ClientOptions::HostPort("another_notlocalwronghost"), // wrong host | |
correct_host_port, | |
ClientOptions::HostPort("localhost", 9001), // wrong port | |
ClientOptions::HostPort("1127.911.2.2"), // wrong host | |
}) | |
.SetPingBeforeQuery(true)); | |
ClientOptions::Endpoint correct_endpoint{"localhost", 9000}; | |
Client client(ClientOptions() | |
.SetEndpoints({ | |
{"localhost", 8000}, // wrong port | |
{"localhost", 7000}, // wrong port | |
{"1127.91.2.1"}, // wrong host | |
{"1127.91.2.2"}, // wrong host | |
{"notlocalwronghost"}, // wrong host | |
{"another_notlocalwronghost"}, // wrong host | |
{correct_endpoint}, | |
{"localhost", 9001}, // wrong port | |
{"1127.911.2.2"}, // wrong host | |
}) | |
.SetPingBeforeQuery(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add proper unit-tests and fix highlighted issues.
@DF5HSE ping? |
@Enmk hi, I have just started to continue working on this pr. Hope it will be ready soon |
Hi! |
@Enmk ping? |
It is possible to specify multiple hosts to connect to. It approves fault tolerance of connection because when one host isn't alive client can try to connect to another one.
The connection will be set to the first available host.
When
ResetConnection
is called, it tries to connect to the first host. If connection failed, then it tries to connect to the next host in list, while list doesn't end up (round-robin approach). If list ends up,ResetConnection
throws exception of last host.